Separate the act of resolving vs loading a dataset - #717

Before you submit for review:

• Does your PR follow guidelines from CONTRIBUTIONS.md?
• Did you summarize what this PR does clearly and concisely?
• Did you include performance data for changes which may be performance impacting?
• Did you include useful docs for any user-facing changes or features?
• Did you include useful javadocs for developer oriented changes, explaining new concepts or key changes?
• Did you rebase your branch onto the latest main for regression testing and PR submission?
• Did you trigger regression testing via Run Bench Main and review results?
• Did you adhere to the code formatting guidelines (TBD)
• Did you group your changes for easy review, providing meaningful descriptions for each commit?
• Did you ensure that all files contain the correct copyright header?
• Did you add documentation for this feature to the release notes directory?

If you did not complete any of these, then please explain below.

@ashkrisk you might want to rebase this to main

I think the code looks fine but it feels like we're losing some flexibility in swapping out the Supplier function for the new DataSetFiles class. Would it not be possible to have DataSetFiles implement Supplier to achieve the same thing?

Would it not be possible to have DataSetFiles implement Supplier to achieve the same thing?

The goal is to be able to use the same DataSetFiles instance to eventually load two different types of datasets - the current one which loads everything into memory and a future one which uses memory-mapping instead.

If DataSetFiles were to implement Supplier (and DataSetInfo treats it as a Supplier without knowledge of the underlying type), we are forced to pick one of those two access methods. That inflexibility is what this PR aims to remove.

I agree there are trade-offs associated with this. As I mentioned in the PR description there are alternatives that retain both kinds of flexibility, but I believe such a design is not worth the extra complexity at this point in time (and can always be added later if we really need it).

@MarkWolters looking at the version of the DataSetInfo class from the last PR in this stack may help explain what I'm going for here. Specifically, there are two methods, getDataSet and getMappedDataSet, and the benchmark utility can pick the one it needs.

https://github.com/datastax/jvector/blob/6a304ba6d084324776a475ca327439ee7876e0e6/jvector-examples/src/main/java/io/github/jbellis/jvector/example/benchmarks/datasets/DataSetInfo.java

Yeah, I see where you're going with it and I think it's a worthwhile effort, I just don't like how it's locked into Fvec/Ivec as the data format. I think it would be fairly cheap and easy to implement DataSetFiles as an interface that defines getBasePath(), getQueryPath() etc. without locking it in to one format, while providing the fvec or hdf5 or whatever implementation as a concrete class.

It's not possible to implement DataSetFiles as a class with methods like getBasePath() or getQueryPath() without loss of generality. Consider that HDF5 datasets may have just a single file containing all three facets within it.

The change I've just pushed preserves generality by going in a slightly different direction - DataSetInfo is turned into an interface of which DataSetInfoMFD is an implementation. We could theoretically extend it using a DataSetInfoHDF5 class in the future if required.

@MarkWolters what do you think?

I like that approach, thanks for being responsive and flexible!

It seems to me that this PR is trying to solve a problem which has already been solved. I'll explain below:

DataSet is already abstract before this. This is intentional, as this subsumes the modal behavior of how a dataset should be loaded. There are different implementations of DataSet to illustrate this.

DataSetProperties captures the ancillary/metadata elements in a contract type as well. This is how we provide the information about a dataset without having to load it or bind the metadata access to the dataset access. This what allows us to know about a dataset without having to have a dataset.

DataSetInfo is intentionally not a contract type, because it serves multiple purposes:

1. To be a caching handle for both datasets and dataset properties. When you have a DataSetInfo reference, you indirectly have knowledge of the dataset whether or not you have transferred or loaded it yet.
2. To provide a useful and stable wrapper type that allows memoization and access to these contract types, and associated helper logic which should be centralized and uniform for all DataSet and DataSetProperty tuples.

As such, making DataSetInfo into a contract type was only forced in the changes in this PR when the interior dataset type for a dataset was changed to a concrete type in 290f698

So, practically speaking, and with specific design support, the act of resolving and loading datasets was already provided for with appropriate contract types, and with these changes, the necessary convenience of having a handle which describes dataset properties without loading it first has been removed [ actually it hasn't, as DataSetInfo IS-A DataSetProperties] . This will not work well for enumerating and planning multi-dataset test operations, and forcing the metadata view into the same lifecycle as the dataset loaded view is not something we should regress.

[EDIT...]
I'm not against adding what we need here. Looking forward to discussing it.

@jshook I think there's a disconnect between your understanding of the purpose of this PR and my understanding. I suspect the breakup of this change into a set of four stacked PRs is partially to blame. I'll try to explain below.

First let me clarify some definitions:

• By Resolving a dataset, I'm talking about browsing the available catalogs to identify it's files on disk, potentially downloading those files if needed.
• By Loading a dataset, I'm referring to the process of making the data available to the application as a kind of Java construct, i.e. some implementation of DataSet.

Here is how they map to our existing API:

// Step 1: Resolve the dataset, throw if not available
DataSetInfo dsInfo = DataSets.loadDataSet("cohere-english-v3-100k").orElseThrow();

// Step 2: Load the DataSet
DataSet ds = dsInfo.getDataSet();

However, to create a DataSetInfo in it's current state, you need a Supplier<DataSet>. This means that by the time you obtain dsInfo, the choice of how to load the dataset has already been made.

The goal of this PR series is to allow BenchYAML to do either of the following based on configuration

DataSet inMemDs = dsInfo.getDataSet();
// or
DataSet mmapDs = dsInfo.getMappedDataSet();

Now, we can debate about the best way to achieve this, but I hope we can agree at the very least that the current interface does NOT solve this problem.

Please do let me know if there's a subtlety here that I've missed.